Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(operators): add mapResponse #4302

Merged
merged 9 commits into from
May 13, 2024
Merged

feat(operators): add mapResponse #4302

merged 9 commits into from
May 13, 2024

Conversation

tom9744
Copy link
Contributor

@tom9744 tom9744 commented Apr 17, 2024 •
edited
Loading

PR Checklist

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

export const loadAllUsers = createEffect((
  actions$ = inject(Actions),
  usersService = inject(UsersService)
) => {
  return actions$.pipe(
    ofType(UsersPageActions.opened),
    exhaustMap(() => {
      return usersService.getAll().pipe(
        map((users) => UsersApiActions.usersLoadedSuccess({ users })),
        catchError((error) =>
          of(UsersApiActions.usersLoadedFailure({ error }))
        )
      );
    })
  );
});

Closes #4230

What is the new behavior?

import { mapResponse } from '@ngrx/operators';

export const loadAllUsers = createEffect((
  actions$ = inject(Actions),
  usersService = inject(UsersService)
) => {
  return actions$.pipe(
    ofType(UsersPageActions.opened),
    exhaustMap(() => {
      return usersService.getAll().pipe(
        mapResponse({
          next: (users) => UsersApiActions.usersLoadedSuccess({ users }),
          error: (error) => UsersApiActions.usersLoadedFailure({ error }),
        })
      );
    })
  );
});

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I am not 100% sure if I correctly understood the expected behavior of mapResponse, as @markostanimirovic suggested in #4230.

Please feel free to give me feedback.

Thanks.

@netlify Netlify
Copy link

netlify bot commented Apr 17, 2024 •
edited
Loading

✅ Deploy Preview for ngrx-io ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit f09ef97
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/663f8781c9a5320008a5d281
😎 Deploy Preview https://deploy-preview-4302--ngrx-io.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

markostanimirovic
markostanimirovic requested changes Apr 24, 2024
View reviewed changes
Copy link
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @tom9744! In addition to the suggested changes, can you please add the mapResponse section to this page: https://ngrx.io/guide/operators/operators

It should contain the description and example of using the mapResponse operator. The example can be an NgRx effect + mapResponse.

modules/operators/src/map-response.ts Outdated Show resolved Hide resolved
modules/operators/spec/map-response.spec.ts Outdated Show resolved Hide resolved
@markostanimirovic markostanimirovic added the Needs Cleanup Review changes needed label Apr 26, 2024
@tom9744
Copy link
Contributor Author

tom9744 commented Apr 28, 2024

I appreciate your kind feedback on my work, @markostanimirovic!

I modified the code based on your suggestions, and am currently working on writing a document for ngrx.io.

tom9744
tom9744 commented Apr 28, 2024
View reviewed changes
modules/operators/src/map-response.ts Outdated
error: (error: E) => R2;
type MapResponseObserver<T, S, E> = {
next: (value: T) => S;
error: (error: unknown) => E;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would make more sense to type error as unknown because we don't know what type the error can possibly be in various use cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change. When error is explicitly set to unknown, it's not possible to override its type:

mapResponse({
  next: (val) => val + 1,
  error: (err: { message: string }) => err, // compilation error
})

This should be allowed.

@markostanimirovic markostanimirovic removed the Needs Cleanup Review changes needed label Apr 29, 2024
markostanimirovic
markostanimirovic requested changes May 6, 2024
View reviewed changes
Copy link
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tom9744! Few small suggestions 👇

modules/operators/spec/map-response.spec.ts Outdated Show resolved Hide resolved
modules/operators/spec/map-response.spec.ts Outdated Show resolved Hide resolved
modules/operators/src/map-response.ts Outdated
error: (error: E) => R2;
type MapResponseObserver<T, S, E> = {
next: (value: T) => S;
error: (error: unknown) => E;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change. When error is explicitly set to unknown, it's not possible to override its type:

mapResponse({
  next: (val) => val + 1,
  error: (err: { message: string }) => err, // compilation error
})

This should be allowed.

projects/ngrx.io/content/guide/operators/operators.md Outdated Show resolved Hide resolved
@tom9744
Copy link
Contributor Author

tom9744 commented May 11, 2024

I understood that typing the error parameter as unknown provides a developer more accuracy, but less convenience.

@tom9744 tom9744 requested a review from markostanimirovic May 11, 2024 14:50
@tom9744 tom9744 force-pushed the feat/operators/map-response branch from aeb4bdf to f09ef97 Compare May 11, 2024 14:58
markostanimirovic
markostanimirovic approved these changes May 11, 2024
View reviewed changes
Copy link
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@markostanimirovic markostanimirovic changed the title feat(operators): add mapResponse #4230 feat(operators): add mapResponse May 11, 2024
timdeschryver
timdeschryver approved these changes May 13, 2024
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
@markostanimirovic do you think we should also add a JSDoc (with usagenotes) as we do with the other operators?

@timdeschryver timdeschryver merged commit c460920 into ngrx:main May 13, 2024
5 checks passed
@markostanimirovic
Copy link
Member

Thanks! @markostanimirovic do you think we should also add a JSDoc (with usagenotes) as we do with the other operators?

Good point Tim! I created an issue for this: #4336

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timdeschryver timdeschryver timdeschryver approved these changes

@markostanimirovic markostanimirovic markostanimirovic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

@ngrx/operators: add mapResponse
3 participants
@tom9744 @markostanimirovic @timdeschryver